fix: Invoice report title in message preview shows an incorrect report title#78992
Conversation
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Reviewing this proposal. |
|
@mananjadhav @JmillsExpensify Any update on this PR? |
| const isReportArchived = useReportIsArchived(report.reportID); | ||
| const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`, {canBeMissing: true}); | ||
| // Calculating the report title with parent report if parent report is invoice and current report is chat thread. | ||
| const isParentInvoiceAndIsChatThread = useMemo(() => isChatThread(report) && isInvoiceReport(parentReport), [report, parentReport]); |
There was a problem hiding this comment.
This seems repititive? Can we move this to a util please?
There was a problem hiding this comment.
Done.
Moved the repetitive logic from RoomMembersPage, ShareCodePage and ReportDetailsPage to a util function getReportForDisplay.
| const title = getReportName(reportHeaderData, policy, parentReportAction, personalDetails, invoiceReceiverPolicy, undefined, undefined, isReportHeaderDataArchived); | ||
| const subtitle = getChatRoomSubtitle(reportHeaderData, false, isReportHeaderDataArchived); | ||
| // This is used to get the status badge for invoice report subtitle. | ||
| const statusTextForInvoiceReport = isParentInvoiceAndIsChatThread ? getReportStatusTranslation({stateNum: reportHeaderData?.stateNum, statusNum: reportHeaderData?.statusNum, translate}) : undefined; |
There was a problem hiding this comment.
Can you highlight the need for this change please?
| * For invoice chat threads, returns the parent invoice report. | ||
| * For other cases, returns the provided report. | ||
| */ | ||
| function getReportForDisplay(report: OnyxEntry<Report>): OnyxEntry<Report> { |
There was a problem hiding this comment.
Should we name this as getReportForHeader? Considering that's what we're using it for?
There was a problem hiding this comment.
This will be done till 12:15 pm IST. I'll notify here.
|
@suhailpthaj The revised changes look fine. Last one comment, and I'll work on the checklist. Can you please sync the latest main and resolve conflicts? |
…icts and merged main
|
@mananjadhav Synced the latest main and Resolved the merge conflicts. |
| const isPolicyExpenseChat = useMemo(() => isPolicyExpenseChatUtils(report), [report]); | ||
| const backTo = route.params.backTo; | ||
| const isReportArchived = useReportIsArchived(report.reportID); | ||
| const reportForSubtitle = useMemo(() => getReportForHeader(report), [report]); |
There was a problem hiding this comment.
May be I missed earlier, we defined this but I don't see it being used?
| const isTaskReport = isTaskReportReportUtils(report); | ||
| const reportHeaderData = !isTaskReport && !isChatThread && report?.parentReportID ? parentReport : report; | ||
| // This is used to check if the report is a chat thread and has a parent invoice report. | ||
| const isParentInvoiceAndIsChatThread = isChatThread && !!parentReport && isInvoiceReport(parentReport); |
There was a problem hiding this comment.
Possible to simplify this to use util?
There was a problem hiding this comment.
I though this too to move in util but this has another logic too to decide whether to pick the parent report or not and I didn't want to break anything that's why I left HeaderView as it is.
Even if we move this to util then we do need isParentInvoiceAndIsChatThread here as there are two factors to decide whether to pick the parent report or not in HeaderView, other locations has only one.
|
Testing the changes now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-invoice-title.movAndroid: mWeb Chromemweb-chrome-invoice-title.moviOS: HybridAppios-invoice-title.moviOS: mWeb Safarimweb-safari-invoice-title.movMacOS: Chrome / Safariweb-invoice-title.mov |
|
@Beamanator @JmillsExpensify All yours. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@Beamanator @JmillsExpensify Bump for PR review. In the latest commit, I only resolved merge conflicts and fixed prettier formatting. IMO, the code looks good now. |
|
@JmillsExpensify wanna do a review before me? 🙏 |
|
Quick bump on this @JmillsExpensify @Beamanator |
JmillsExpensify
left a comment
There was a problem hiding this comment.
All good on the product side. Apologies for the delay!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.3.5-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.5-7 🚀
|

Explanation of Change
fix:
Fixed Issues
$ #75857
PROPOSAL: #75857 (comment)
Tests
Precondition: You must have the workspace with invoices enabled.
Amount expense.Amount expense.Amount expense.Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-native.mp4
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari